Add versioned benchmark JSON schema validation (v1) - #10
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The schema/tests do not currently enforce the required protocol field per Issue #3 acceptance criteria, so the validation guarantees are incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a versioned (v1) JSON Schema and pytest-based validation to make checked-in benchmark result JSON files machine-checkable and enforce required metadata fields.
Changes:
- Introduces Draft 2020-12 JSON Schema for benchmark results (
schema_version: 1). - Adds tests that validate the checked-in Qwen3 microbenchmark result file against the schema and exercise required-field failures.
File summaries
| File | Description |
|---|---|
| schemas/benchmark-v1.schema.json | Adds the v1 JSON Schema definition for benchmark result files. |
| tests/test_benchmark_schema.py | Adds pytest validation of the checked-in benchmark JSON against the schema plus negative required-field tests. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "required": [ | ||
| "schema_version", | ||
| "claim_scope", | ||
| "model", | ||
| "model_revision", | ||
| "device", | ||
| "runs", | ||
| "summary", | ||
| "kind", | ||
| "prompt_tokens", | ||
| "generated_tokens", | ||
| "warmup_tokens", | ||
| "trials", | ||
| "budget", | ||
| "buffer_size", | ||
| "seed" | ||
| ], |
| schema = load_json(SCHEMA_PATH) | ||
| data = load_json(QWEN_RESULT_PATH) | ||
| validator = Draft202012Validator(schema) | ||
| errors = sorted(validator.iter_errors(data), key=lambda e: list(e.path)) | ||
| assert errors == [], [e.message for e in errors] |
| REQUIRED_FIELDS = [ | ||
| "schema_version", | ||
| "claim_scope", | ||
| "model", | ||
| "model_revision", | ||
| "device", | ||
| "runs", | ||
| "summary", | ||
| "kind", | ||
| "prompt_tokens", | ||
| "generated_tokens", | ||
| "warmup_tokens", | ||
| "trials", | ||
| "budget", | ||
| "buffer_size", | ||
| "seed", | ||
| ] |
|
Hi DaBestCode, could you please approve workflows for this PR? Thank you! |
|
Addressed Copilot review feedback in latest commits:
Local checks pass:
Could a maintainer please approve workflows so remaining CI can run? Thanks! |
|
could a maintainer please approve workflows for this PR so CI can continue? Thank you! |
DaBestCode
left a comment
There was a problem hiding this comment.
Thanks for taking this on. The schema direction is useful, but this needs four focused fixes before merge:
- Please remove the newly added top-level
protocolfield from both the fixture and schema. The issue guidance explicitly defines the protocol through the existing required fields (kind, token counts, trials, budget, buffer, and seed) and asks not to change the producer format solely to add a key. - Add
jsonschemato thetestextra inpyproject.toml. In a clean project environment,pytest -qcurrently fails during collection withModuleNotFoundError: No module named 'jsonschema'. - Add the schema compatibility policy to
results/README.md: additive optional fields remain compatible within v1; removing/renaming fields or changing their semantic meaning requires a new schema version. - Run
ruff formatontests/test_benchmark_schema.py;ruff format --check .currently fails on the final assertion.
Verified locally: ruff check . and mypy src pass; formatting and pytest fail for the reasons above. Please rerun the complete CONTRIBUTING.md check list after updating.
…add compatibility policy, and format tests
|
hanks @DaBestCode! All four points have been addressed in commit 4fcae73: |
DaBestCode
left a comment
There was a problem hiding this comment.
Thanks for addressing all four prior findings. I reran the complete CONTRIBUTING.md suite locally; 34 tests and the package build pass.
There is one remaining schema-compatibility issue before approval:
RandKVConfigand both CLIs allowbuffer_size=0, but the schema usesminimum: 1, so it rejects a valid benchmark result. Please change this tominimum: 0.budgetis an integer inRandKVConfigandargparse, but the schema declares it asnumber, which accepts fractional values the producer cannot emit. Please usetype: integerwithminimum: 1.
Please add a small regression test showing a zero buffer validates and a fractional budget does not. Everything else from the previous review is resolved.
|
Thanks @DaBestCode! Addressed in commit eda72c7: |
DaBestCode
left a comment
There was a problem hiding this comment.
Approved. The latest commit resolves the remaining schema compatibility findings: zero-sized recency buffers validate, budgets are constrained to positive integers, and both cases have regression coverage. Verified locally with 36 tests plus Ruff, formatting, mypy, package build, and diff checks; the hosted Python 3.10/3.13 matrix and GitGuardian also pass.
Fixes #3
Summary
This PR introduces schema-based validation for benchmark result JSON files.
Changes
schemas/benchmark-v1.schema.jsontests/test_benchmark_schema.pySchema/versioning policy
Validation run
ruff check .ruff format --check .mypy srcpytest -qAll checks pass locally.